-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Thrift 2905 & - Modern Objective-C & Swift Support #539
Conversation
• Use NSError based error & exception handling • Use NS_ENUM enumerations (with standard format) • nullable & nonnull attributes for parameters • Swift interoperability (nullability, enums & error handling) • Removed instance variables from public header • Remove retain/release stubs • Remove all deallocs
The current message name & source file/line information is added to the protocol error that wraps the transport error.
…or the top level object already.
…ontainer entries)
Allows implementing "description" by the application via category/extension
Was moved to TNSStreamTransport to make a general close method & keep streams private. Instead we want the streams public and so the close method is moved back to its origin.
The new method uses SecTrustEvaluate on the stream's internal trust object. Removes the deprecation warning given by previous implementation.
…ose method in TNSStream
All customization should be done via the NSURLSession & NSURLSession config. This should be the preferred method so we enforce that by disallowing any customization of the request before usage.
All helper classes (e.g. _args & _result) are generated with the service name included. This fixes the issue with duplicate helper class names.
It could be as low as 6.0 but with 90% of the world on iOS 8. I don't think we need to worry about that.
Re cocoa/objc/swift: I see. Leave it as it is. Question: Are the eror codes from TTransportError.h (and mybe others) only used internally or are they visible to the other side? If they are public, they probably should match the usual Thrift error numbers, like in this C++ code, otherwise a (for example) C++ client will have a hard time dealing with them. |
Client: Cocoa Patch: Kevin Wooten <kevin@wooten.com> This closes apache#539
With regard to the C++ and Cocoa exception codes matching... It doesn't seem like the exceptions are being thrown in the same circumstances in most cases. Maybe we should look at standardizing these codes in general? |
Client: Cocoa Patch: Kevin Wooten <kevin@wooten.com> This closes apache#539
Good idea :-) In fact, the exception codes are already standardized for all standard Thrift exceptions ( |
The application, protocol & transport exceptions are now reported in much the same conditions (and with same error code) as with other language bindings. Java & CPP were used as examples of when to throw the correct exceptions.
I've brought the exception reporting in line with other languages. I looked into Java & C++ to see the error codes they for all. Historically TApplicationError seems to have been correct in the past and I trampled on it by changing the codes themselves. The protocol & transport errors needed a bit of shuffling but now they are using a meaningful code in the correct context. I've kept some of the extended error information we were traditionally reporting but put it into variables that do not escape the current environment. |
First, thanks for all the modernization!
|
@jriskin Also, what was the reason for needing to delete the swift files? Are you targeting an platform version that doesn't support it? |
output header: @Property (assign, nonatomic) SInt32 push_id;
@Property (strong, nonatomic) NSMutableArray * post_ids;
@Property (assign, nonatomic) SInt32 post_limit;
@Property (assign, nonatomic) SInt64 bookmark;
@Property (assign, nonatomic) SInt32 comment_limit;
|
I assume from the way the IDL was pasted that all of the
In Objective-C selectors are explicitly not scoped to their class. This has surprised me more times than I would like to believe. |
From the code it difficult to see what's the problem. Yes, in objc selectors are separate from the classes - they're shared between them and stored as plain strings. That creates problems only when you're dealing with |
@creker ah, that makes some sense, I am using id for a couple of types of objects that I store in arrays and they don't resolve to a type until runtime. I'll revert all the code once i'm in a stable state and see if i can figure it out, for now i just renamed the conflicts. Also, is it an issue that they are using 'id' as a variable name? That seems awfully unsafe in obj-c. I may push to see if i can get them to change this to post_id or something more reasonable. e.g. |
Had a few problems with a field named |
I completely agree, definitely going to make the case to have the team defining the thrift to change... i would think it might be a good idea in the generator to automatically prefix reserved keywords as an option when the programmer can't control the thrift. |
It also might be nice to use NSNumber objects, would solve the issue of storing them in NSArrays. |
You read my mind. Was thinking about both features you mention - somehow deal with conflicting names (shouldn't be too much of them) and store all value types in NSNumber. But latter will cause the fields to loose their type. Still, compiler options for both of them would be nice. |
The issue with the |
FWIW, we currently use a property named "id". I'll admit to being concerned when we first started using it but beyond looking awkward it has produced no issues. |
Awesome! I'll check it out now. FYI, coercing the type solved my namespace issue. e.g. Thanks for the quick fixes! |
…ective-C Client: Cocoa (ObjectiveC & Swift) Author: Kevin Wooten <kevin@wooten.com> This closes apache#539
…ective-C Client: Cocoa (ObjectiveC & Swift) Author: Kevin Wooten <kevin@wooten.com> This closes apache#539
…ective-C Client: Cocoa (ObjectiveC & Swift) Author: Kevin Wooten <kevin@wooten.com> This closes apache#539
This pull request is really just to start a conversation about how to move forward with the changes I've made to the Thrift Cocoa library and generator.
The nature of the changes are that they are big & a bit destructive. Unfortunately there wasn't much getting around that. Hopefully working with the community we can agree on the best way to integrate these changes. Maybe just as another "language" binding instead of replacing the current "cocoa".
Goals
Major Changes
• Addes Swift code generator
Why?
We made some very large changes. Here's why...
NSException to NSError
The use of NSException has been very problematic for us. Aside from the direction of Apple, which is to use exceptions for "fatal errors" only, it comes with a host of issues. First, Clang still produces incorrect code in many cases when using exceptions. Take this code for example...
Clang should produce an error but doesn't. This is only a nuisance though, much worse are the ARC failures we've found when catching exceptions up the stack. Secondly, Swift has no support for exceptions (and with Swift 2 we've seen it won't ever use NSException).
It's for these reasons we changed the the Cocoa library to use NSError, all of the generated (synchronous) methods now follow the Apple guidelines and return a BOOL or object pointer with a last error parameter of type NSError**; return values of NO or nil mean the error parameter is populated and the call failed (or and exception was thrown from the server). These changes have been made throughout the generated and library code.
Client Method Mapping
NSError & Swift 2
These changes conveniently take the form of Swift 2's error handling system. This means that protocols and transports can be used from and written in Swift and will be compatible with the current & future directions of Swift.
ARC Only
ARC is here to stay; Apple deprecated everything else a few years ago. All code related to maintaining backwards compatibly with non-ARC mode has been removed.
Mutl-Threaded Asynchronous Transports
The current implementations means that clients cannot be used in different threads. This means the user is left to synchronize access to them or, as in our case, build a client per thread to ensure parallelism.
Our changes have reengineered the way asynchronous works. Now, asynchronous clients accept only protocol and async-transport factories. Each call allocates a new transport and protocol binds them before use. This provides the best options for parallelism with a fairly small overhead.
A note on performance... the alternative approaches, which we used in the past, required thread local storage inside the transport which meant an NSDictionary lookup (i.e. threadDictionary) for each read/write/flush. The new approach only requires 2 simple object allocations. Much improved.
NSURLSession (NSURLConnection is now deprecated)
The new THTTPSessionTransport replaces the old THTTPClient. It's both asynchronous and uses the new NSURLSession classes for conformance in the new iOS/OSX versions.
NS_ENUM & Swift
The generated code now generates properly formatted enumerations using the NS_ENUM macro & no underscores. This is to ensure they work with code completion in objective-c and are imported into Swift with the best possible syntax.